feat: adds null migrator #31
Conversation
sam-berning
force-pushed
the
null-migrator
branch
from
acef7e6 to
69a2377
Compare
| if models.len() != 1 { | ||
| return Err(NullMigratorError::TooManyModelVersions); | ||
| } |
There was a problem hiding this comment.
| if models.len() != 1 { | |
| return Err(NullMigratorError::TooManyModelVersions); | |
| } | |
| snafu::ensure!(models.len() == 1, NullMigratorError::TooManyModelVersionsSnafu); |
| define_model!(NullModelB, "v2"); | ||
| #[test] |
There was a problem hiding this comment.
I would separate these with some whitespace as a readability thing.
| use serde::{Deserialize, Serialize}; | ||
|
|
||
| use super::common::define_model; |
There was a problem hiding this comment.
We tend not to separate use statements with whitespace, and let rustfmt handle ordering.
The exception to this is pub use statements for shaping the interface.
|
We'll want to increment the version and tag the repo appropriately so that we can pull this change into Bottlerocket. Here's the commit where that was done in the last version bump. |
sam-berning
force-pushed
the
null-migrator
branch
from
69a2377 to
37ecfd5
Compare
New version should be |
sam-berning
force-pushed
the
null-migrator
branch
3 times, most recently
from
66c4a65 to
4915376
Compare
Yes! |
| #[snafu(visibility(pub))] | ||
| pub enum NullMigratorError { |
There was a problem hiding this comment.
@cbgbt We should strongly consider not using Snafu for pub errors. The long-and-short of it is that we are exposing a library and its traits in our public interface and it might be impossible to avoid a major version break if the Snafu library changes in some way. Can discuss.
For this PR it's fine, but I would recommend a backlog item to convert away from pub Snafu errors in favor of hand-rolled errors (they're simple, it's not a lot of boilerplate).
There was a problem hiding this comment.
Good suggestion. I've cut #32 to track this idea.
| @@ -0,0 +1,201 @@ | |||
| use anyhow::Result; | |||
There was a problem hiding this comment.
Instead of these filenames, I would go with
bottlerocket-settings-sdk/tests/migration_validation/linear.rs
bottlerocket-settings-sdk/tests/migration_validation/null.rs
| assert!(matches!( | ||
| NullMigratorExtensionBuilder::with_name("null-migrator") | ||
| .with_models(vec![BottlerocketSetting::<NullModelA>::model()]) | ||
| .build(), | ||
| Ok(_) | ||
| )); |
There was a problem hiding this comment.
Or
| assert!(matches!( | |
| NullMigratorExtensionBuilder::with_name("null-migrator") | |
| .with_models(vec![BottlerocketSetting::<NullModelA>::model()]) | |
| .build(), | |
| Ok(_) | |
| )); | |
| assert!( | |
| NullMigratorExtensionBuilder::with_name("null-migrator") | |
| .with_models(vec![BottlerocketSetting::<NullModelA>::model()]) | |
| .build() | |
| .is_ok() | |
| ); |
Or (this is probably best because the Error is shown in the panic text)
NullMigratorExtensionBuilder::with_name("null-migrator")
.with_models(vec![BottlerocketSetting::<NullModelA>::model()])
.build()
.unwrap();|
|
||
| #[test] | ||
| fn test_multiple_models() { | ||
| assert!(matches!( |
There was a problem hiding this comment.
Same as above, can use is_err()
Signed-off-by: Sam Berning <[email protected]>
Signed-off-by: Sam Berning <[email protected]>
sam-berning
force-pushed
the
null-migrator
branch
from
63df59d to
09be241
Compare
Issue #, if available: #30
Description of changes:
Adds a
NullMigratorthat can be used with settings with only one version. This removes some of the boilerplate when developing a new extension.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.